feat: add optional native-tls support as alternative to rustls#8037
feat: add optional native-tls support as alternative to rustls#8037r0x0d wants to merge 3 commits intoblock:mainfrom
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ad16ab9720
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 59bc733759
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3049f47a47
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
jamadeo
left a comment
There was a problem hiding this comment.
This makes sense to me and thank you for taking it on!
My main question is whether this should be two features or one. With both default-tls/native-tls features, we now have a third state (assuming we keep them mutually exclusive) where neither are enabled, and I'm not sure that would ever produce a valid build (?).
If that's true I'd imagine we'd want a single new feature, native-tls, and when this is off, use what we're calling here default-tls.
|
@jamadeo I think it's okay for Arguably we should make it exactly one, and add a similar gate for an error when more than one is enabled; an alternative is to allow specifying both and selecting the implementation at runtime, but that seems overly complex and I don't know what the use case would be. I think I'd prefer |
Add a `native-tls` Cargo feature across all crates as an alternative to the default `rustls` TLS backend. This enables building goose against the platform's native TLS stack (OpenSSL on Linux, Secure Transport on macOS, SChannel on Windows), which is required for Fedora/RHEL packaging where vendoring rustls/aws-lc-rs is problematic. Changes by crate: goose (core): - Add `default-tls` and `native-tls` feature groups gating reqwest, rmcp, sqlx, jsonwebtoken, and oauth2 TLS backends - Set reqwest, rmcp, sqlx, oauth2 to default-features = false so TLS backend is selected purely via features - Switch jsonwebtoken to default-features = false with explicit use_pem; gate aws_lc_rs (default-tls) vs rust_crypto (native-tls) - Conditionally compile Identity::from_pem (rustls) vs Identity::from_pkcs8_pem (native-tls) in api_client.rs goose-server: - Add `default-tls` and `native-tls` feature groups gating reqwest, tokio-tungstenite, axum-server, rustls, aws-lc-rs, and openssl - Make rustls, aws-lc-rs optional (default-tls); add openssl optional (native-tls) - Refactor tls.rs: extract generate_self_signed_cert() and sha256_fingerprint() helpers; use cfg-gated TlsConfig type alias (RustlsConfig vs OpenSSLConfig) - Cfg-gate rustls crypto provider initialization in agent.rs and lapstone.rs - Cfg-gate axum_server::bind_rustls vs bind_openssl in agent.rs - Add TLS integration tests in tests/tls_test.rs goose-cli: - Add `default-tls` and `native-tls` feature groups gating reqwest and sigstore-verification TLS backends, forwarding to goose and goose-mcp goose-mcp: - Add `default-tls` and `native-tls` features gating reqwest TLS backend; remove hardcoded rustls feature from reqwest dep goose-acp: - Add `default-tls` and `native-tls` feature forwarding to goose dep CI and build: - Remove scripts/check-no-native-tls.sh (no longer applicable) - Remove banned TLS crate check from ci.yml and Justfile Usage: # Default build (rustls, unchanged behavior) cargo build # Build with native-tls (OpenSSL) cargo build --no-default-features --features native-tls,code-mode Co-authored-by: Claude <noreply@anthropic.com> Signed-off-by: Rodolfo Olivieri <rolivier@redhat.com> Made-with: Cursor
Propagate TLS feature selection from goose-acp to goose-mcp so standalone goose-acp-server builds retain HTTPS support for builtin extensions. Use openssl to convert any PEM key format (PKCS#1, SEC1, PKCS#8) to PKCS#8 in the native-tls path, so existing mTLS setups with traditional RSA or EC keys continue to work. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Rodolfo Olivieri <rolivier@redhat.com>
…itation Change all rustls cfg guards from `not(feature = "native-tls")` to `feature = "default-tls"` so the rustls path does not compile when neither TLS backend is enabled (e.g. --no-default-features). Gate the tls module and its consumers on having at least one TLS backend, and return a clear error if TLS is requested at runtime without one. Add module-level documentation to tls.rs explaining that the native-tls feature uses OpenSSL for the server listener via axum-server. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Rodolfo Olivieri <rolivier@redhat.com>
3049f47 to
76def0c
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 76def0c909
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
|
||
| [features] | ||
| default = ["code-mode", "local-inference"] | ||
| default = ["code-mode", "local-inference", "rustls-tls"] |
There was a problem hiding this comment.
Make goose native-tls replace the default rustls backend
When a downstream crate opts into goose's new native-tls feature in the normal Cargo way (features = ["native-tls"]), line 11 still keeps rustls-tls in the default set, so both TLS features become active. That now trips the mutual-exclusion compile_error! in crates/goose/src/lib.rs:1-5, which means the advertised alternative backend is unusable for consumers unless they also know to add default-features = false to their dependency spec.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Hmm, thinking about this more, this means that if any one crate in the dependency tree doesn't disable default features on goose, you can't switch to native-tls without getting a compile error.
Hot take: What if we just didn't have any default features (everything is opt-in)?
|
@jh-block makes sense to me -- you're right I had forgotten the strictly additive aspect. Agreed that |
Summary
Add a
native-tlsCargo feature across all crates as an alternative to the defaultrustlsTLS backend. This enables building goose against the platform's native TLS stack (OpenSSL on Linux, Secure Transport on macOS, SChannel on Windows), which is required for Fedora/RHEL packaging where vendoring rustls/aws-lc-rs is problematic.Changes by crate:
goose (core):
default-tlsandnative-tlsfeature groups gating reqwest, rmcp, sqlx, jsonwebtoken, and oauth2 TLS backendsgoose-server:
default-tlsandnative-tlsfeature groups gating reqwest, tokio-tungstenite, axum-server, rustls, aws-lc-rs, and opensslgoose-cli:
default-tlsandnative-tlsfeature groups gating reqwest and sigstore-verification TLS backends, forwarding to goose and goose-mcpgoose-mcp:
default-tlsandnative-tlsfeatures gating reqwest TLS backend; remove hardcoded rustls feature from reqwest depgoose-acp:
default-tlsandnative-tlsfeature forwarding to goose depCI and build:
Usage:
Default build (rustls, unchanged behavior) cargo build
Build with native-tls (OpenSSL) cargo build --no-default-features --features native-tls,code-mode
Made-with: Cursor
Testing
Tested by running
cargo testlocally and verifying that native-tls was being used, both with default features and without it.Related Issues
Relates to #ISSUE_ID
Discussion: LINK (if any)
Screenshots/Demos (for UX changes)
Before:
After: